Workaround to Windows filepath.EvalSymlinks() known issues #46
Conversation
WalkthroughThis pull request converts an indirect dependency in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Function Caller
participant Resolver as internal.ResolvePath
participant Platform as OS-Specific Resolver
Caller->>Resolver: Invoke ResolvePath(filename)
alt Unix
Resolver->>Platform: Call filepath.EvalSymlinks(filename)
else Windows
Resolver->>Platform: Call windows.GetFinalPathNameByHandle(file handle)
end
Platform-->>Resolver: Return resolved path or error
Resolver-->>Caller: Return result
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Definitions (1)internal/resolve_path_windows.go (1)
🔇 Additional comments (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/resolve_path_windows.go (1)
1-33: Test edge cases with very long file paths.This function relies on
syscall.MAX_PATH, which may not handle extended Windows paths. Consider dynamic buffer sizing to accommodate paths beyondMAX_PATH.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
go.mod(1 hunks)internal/path.go(1 hunks)internal/resolve_path_unix.go(1 hunks)internal/resolve_path_windows.go(1 hunks)update.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
internal/resolve_path_unix.go (1)
internal/resolve_path_windows.go (1) (1)
ResolvePath(13:32)
internal/resolve_path_windows.go (1)
internal/resolve_path_unix.go (1) (1)
ResolvePath(10:17)
🔇 Additional comments (4)
go.mod (1)
14-14: Confirm the chosen version.Please verify whether
golang.org/x/sys v0.29.0is up-to-date and free of known issues.internal/resolve_path_unix.go (1)
1-17: Straightforward Unix path resolution.This is a clean wrapper around
filepath.EvalSymlinks. Looks good.internal/path.go (1)
14-14: Unified path resolution is a good approach.Switching to
ResolvePathensures consistent cross-platform behavior. Looks good.update.go (1)
57-62: Great replacement for Windows reliability
Switching fromfilepath.EvalSymlinkstointernal.ResolvePathaddresses known issues on Windows and retains behavior for Unix-like systems.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
=======================================
Coverage 77.07% 77.07%
=======================================
Files 28 28
Lines 1435 1435
=======================================
Hits 1106 1106
Misses 279 279
Partials 50 50
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for this PR! It looks good to me. For my understanding, can you explain how to replicate this bug? |
Unfortunately I cannot repro this myself as I develop on macOS. This was a problem my colleague (the attached system info is from their laptop) was facing after having installed our CLI app (with Scoop) and trying to update it with the self-update feature. We quickly narrowed the issue down to I wish I could be of more help but we really never nailed down which particular detail in the system made it fail. Searching for relevant issues in the golang repo itself didn't help much, either. The main takeaway I learned from there was just to avoid using |
|
Thanks for the information! As a side note, I usually disable self-updating when installing from package managers. They can get annoyed when they find out the binary has changed 😆 |
|
So, after some investigation, I can see scoop is using a junction to link the current directory to the latest version. So far so good: I thought: oh well that's going to be easy to replicate in a test. As a matter of fact, yours also works fine so I'll accept the PR 👍🏻 Thank you 😉 |
2 participants
We're using
go-selfupdatev.1.4.1 in our CLI project and the self-update fails on Windows with the following configuration:(not visible in the image, but there's 64GB of RAM on the system)
The error originates from
selfupdate.ExecutablePath()and more specifically, from a call tofilepath.EvalSymLinks()within. This function appears to be notoriously unreliable on Windows, see e.g. golang/go#42201This PR introduces a new internal function
internal.ResolvePath()as a replacement for directfilepath.EvalSymlinks()calls. The implementation usesx/sys/windows.GetFinalPathNameByHandle()on Windows, andfilepath.EvalSymlinks()on unix(-like) platforms.Summary by CodeRabbit
Chores
New Features